Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix image-create with ACLs. Fixes #1394. #1395

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Fix image-create with ACLs. Fixes #1394. #1395

merged 1 commit into from
Aug 17, 2023

Conversation

dhbaird
Copy link
Contributor

@dhbaird dhbaird commented Aug 7, 2023 •
edited
Loading

Relevant Issue (if applicable)

Fixes:
#1394

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@dhbaird dhbaird requested a review from a team as a code owner August 7, 2023 23:35
@dhbaird dhbaird requested review from bergwolf, adamqqqplay and hsiangkao and removed request for a team August 7, 2023 23:35
@anolis-bot
Copy link
Collaborator

@dhbaird , a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/89576

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1395 (67c8c88) into master (34ee825) will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
+ Coverage   46.05%   46.30%   +0.24%     
==========================================
  Files         122      122              
  Lines       38077    38077              
  Branches    38077    38077              
==========================================
+ Hits        17538    17632      +94     
+ Misses      19618    19519      -99     
- Partials      921      926       +5
Files Changed Coverage Δ
rafs/src/metadata/layout/mod.rs 93.67% <100.00%> (ø)

... and 9 files with indirect coverage changes

@anolis-bot
Copy link
Collaborator

@dhbaird , The CI test is completed, please check result:

Test CaseTest Result
build rust golang image✅ SUCCESS
compile nydusd✅ SUCCESS
compile ctr remote✅ SUCCESS
compile nydus snapshotter✅ SUCCESS
run container with rafs✅ SUCCESS
run container with zran✅ SUCCESS
run container with rafs and compile linux✅ SUCCESS

Congratulations, your test job passed!

imeoer
imeoer reviewed Aug 8, 2023
View reviewed changes
rafs/src/metadata/layout/mod.rs
@@ -267,7 +267,7 @@ impl RafsXAttrs {
return Err(einval!("xattr key/value is too big"));
}
for p in RAFS_XATTR_PREFIXES {
if buf.len() > p.as_bytes().len() && &buf[..p.as_bytes().len()] == p.as_bytes() {
if buf.len() >= p.as_bytes().len() && &buf[..p.as_bytes().len()] == p.as_bytes() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixup, it seems we need also to handle this?
https://github.com/dragonflyoss/image-service/blob/34ee8255dea73819f4d634f83260d1fa4fe68c97/rafs/src/metadata/layout/v6.rs#L2088

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps so; I am not 100% sure. I apologize that I have yet produced a test in bats to correspond to testing this. Thank you for the time and consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*That I have not yet produced, I mean!

Copy link
Collaborator

@imeoer imeoer Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dhbaird, you can add a related test case here, it will be used by the smoke test automatically.
https://github.com/dragonflyoss/image-service/blob/34ee8255dea73819f4d634f83260d1fa4fe68c97/smoke/tests/texture/layer.go#L89

@imeoer
Copy link
Collaborator

imeoer commented Aug 8, 2023 •
edited
Loading

@dhbaird Please also add a signed-off-by for the commit by following https://github.com/dragonflyoss/image-service/pull/1395/checks?check_run_id=15694892551

@dhbaird
Copy link
Contributor Author

dhbaird commented Aug 9, 2023

Thanks for the guidance @imeoer! I confirmed that the line you indicated above should change: https://github.com/dragonflyoss/image-service/blob/34ee8255dea73819f4d634f83260d1fa4fe68c97/rafs/src/metadata/layout/v6.rs#L2088

I am working on the tests now and double checking the changes, and will also added signed-off-by.

@dhbaird
Copy link
Contributor Author

dhbaird commented Aug 10, 2023

Hi again @imeoer, When I tried to update the test indicated in previous comments, I got blocked by a new issue: toOCITar() drops all xattrs except for security.capability:

https://github.com/dragonflyoss/image-service/blob/34ee8255dea73819f4d634f83260d1fa4fe68c97/smoke/tests/tool/layer.go#L119

I read the code and traced it to containerd/archive, where it appears to ignore all xattrs except for security.capability:

https://github.com/containerd/containerd/blob/70a2c95ae8c02d7a4e448f0e4fb8bb0e6344b5c7/archive/tar.go#L644

Does this look like an issue in containerd/archive now too? Would you recommend that I create an issue over there too?

Thanks!

@imeoer
Copy link
Collaborator

imeoer commented Aug 11, 2023

Does this look like an issue in containerd/archive now too? Would you recommend that I create an issue over there too?

@dhbaird It seems only the security.capability xattr is supported in OCI tar, so we can't add the test case into the build of oci tar -> nydus rafs, but still can support it in the build of directory -> nydus rafs.

But to keep it simple, I think we just need to add some unit tests for RafsXAttrs, what do you think?

@dhbaird dhbaird force-pushed the bug/image-create-with-acls branch 2 times, most recently from 7d62fdf to a46e709 Compare August 16, 2023 04:49
@dhbaird
Copy link
Contributor Author

dhbaird commented Aug 16, 2023

Hi @imeoer, the code is again ready for review. May you restart the "nydus-unit-test-coverage" automation? Thanks for your helpful feedback and time so far. Any ideas / suggestions / criticisms are welcome!

imeoer
imeoer approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@imeoer imeoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dhbaird for the excellent tests and fixup!

@imeoer imeoer merged commit 156ba6a into dragonflyoss:master Aug 17, 2023
21 of 22 checks passed
@imeoer imeoer mentioned this pull request Aug 17, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imeoer imeoer imeoer approved these changes

@bergwolf bergwolf Awaiting requested review from bergwolf bergwolf is a code owner automatically assigned from dragonflyoss/nydus-reviewers

@adamqqqplay adamqqqplay Awaiting requested review from adamqqqplay adamqqqplay is a code owner automatically assigned from dragonflyoss/nydus-reviewers

@hsiangkao hsiangkao Awaiting requested review from hsiangkao hsiangkao is a code owner automatically assigned from dragonflyoss/nydus-reviewers

Assignees
No one assigned
Labels
anolis_test_pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhbaird @anolis-bot @imeoer